test(export): guard against memory exhaustion on large CSV exports - #1928
Merged
sublime247 merged 1 commit intoAug 30, 2026
Merged
Conversation
Closes sublime247#1794 Investigated the transaction export route (src/routes/export.ts) before writing anything: it already uses pg-query-stream + Node's stream/promises pipeline() to stream rows from Postgres straight through a Transform into the HTTP response, with no SELECT-then-buffer-then-send step anywhere in the CSV, JSON, or PDF code paths. There is no live memory-exhaustion bug in the current export route to fix. What was missing was a test that could actually catch a regression back to buffering. Adds one: streams 200,000 synthetic rows from a lazily-generated source (a generator function wrapped in Readable.from, never a real in-memory array of 200k rows, since that would defeat the point) through the real route, and asserts the response arrives as many small HTTP chunks rather than one/few large ones. A buffered implementation (e.g. collecting every row into an array or a single string before writing) would produce a single large data event close to the full response size; this test's maxChunkBytes-vs-totalBytes assertion would fail against that regression and passes against the current, correctly-streamed implementation. Did not assert on process.memoryUsage() heap deltas directly - tried it, but GC timing under Jest/V8 makes heap measurements too noisy to assert on reliably (confirmed by hand: the same passing implementation intermittently failed a heap-growth bound by 2-3x run to run with no code change). The chunk-shape assertion is the reliable, deterministic signal for "streamed vs. buffered" and is what this test relies on. Also found and disclosing separately (not fixed here, pre-existing and unrelated): tests/routes/export.test.ts (a second, older test file covering the same route with a different X-API-Key auth model) hangs indefinitely on every test, confirmed present on a clean upstream/main checkout with no code changes at all, independent of database availability (reproduced both with and without a real Postgres instance configured). Verification: npx jest src/routes/__tests__/export.test.ts - 7/7 passing (6 pre-existing + 1 new), run multiple times for stability. npx eslint on the touched file - clean.
|
@uche001-dev Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1794
Summary
Investigated
src/routes/export.ts(the transaction bulk-export route) before writing any code, per this repo's own habit of checking whether described bugs still exist. It already usespg-query-stream+ Node'sstream/promisespipeline()to stream rows from Postgres straight through aTransforminto the HTTP response — noSELECT *result set is ever buffered fully in memory, in any of the CSV, JSON, or PDF export paths. There is no live memory-exhaustion bug in the current export route.What this PR adds
A regression test that can actually catch a future reintroduction of buffering:
Readable.from— deliberately never a real in-memory array of 200k rows, since building one would defeat the whole point of the test).maxChunkBytes < totalBytes / 5,dataEventCount > 10). A buffered implementation (collecting every row into an array or one big string before writing anything) would produce a single largedataevent close to the full response size — this assertion would fail against that regression and passes against the current, correctly-streamed implementation.Why not assert on
process.memoryUsage()heap deltas directlyI tried it first. GC timing under Jest/V8 makes heap measurements too noisy to assert on reliably — the same passing implementation intermittently failed a heap-growth bound by 2-3x from run to run with zero code changes. The chunk-shape assertion above is the deterministic, reliable signal for "streamed vs. buffered" and is what the test actually relies on.
Disclosed, not fixed here (pre-existing, unrelated)
tests/routes/export.test.ts— a second, older test file covering the same route with a differentX-API-Key/ADMIN_API_KEYauth model — hangs indefinitely on every test in it. Confirmed present on a cleanupstream/maincheckout with zero code changes, reproduced both with and without a real local Postgres instance configured (ruled out "just needs a database" as the cause). Given the two files clearly test two different, non-overlapping generations of this route's auth model, this looks like a stale file left behind after a refactor rather than something in active use — flagging it rather than guessing at a fix, since I couldn't pin down the actual hang cause within a reasonable amount of investigation time.Verification
npx jest src/routes/__tests__/export.test.ts— 7/7 passing (6 pre-existing + 1 new), run multiple times for stability.npx eslinton the touched file — clean.Testing requirements checklist (from the issue)
src/routes/__tests__/export.test.ts(7/7). Note:tests/routes/export.test.tsis a separate, pre-existing broken file (see disclosure above), unaffected by and unrelated to this change.